eloq_data_store_service/README.md (3)

145-153: Call out prefix removal and object_path requirement inline.

Add a short note under the URL example that bucket_prefix is ignored with URL config and the object_path segment is mandatory (URLs like s3://bucket only will fail validation). This prevents migration surprises.

Apply:

 rocksdb_cloud_s3_url = s3://my-eloqdata-bucket/rocksdb_cloud
+; Note: bucket_prefix is not supported with URL config. If you previously used a prefix,
+; fold it into the bucket name (e.g., s3://prod-my-eloqdata-bucket/rocksdb_cloud).
+; The object_path segment (after the bucket) is required.

---

155-163: MinIO example: add HTTPS and cert note.

Provide an HTTPS variant and a tip that custom CAs/self‑signed certs require proper trust configuration rather than disabling verification in code.

Apply:

 rocksdb_cloud_s3_url = http://localhost:9000/my-bucket/rocksdb_data
+; or, with TLS:
+; rocksdb_cloud_s3_url = https://minio.local:9000/my-bucket/rocksdb_data
+; Ensure the TLS certificate is trusted by the host (install CA if self-signed).

---

286-300: Document strict URL parsing.

Explicitly note that:

• object_path cannot be empty (trailing slash like s3://bucket/ is invalid),
• no leading slash before bucket (s3:///bucket/... is invalid).

Add to Important Notes:

 5. **Validation:** Invalid URLs will cause the application to fail at startup with a descriptive error message.
+6. **URL shape:** The path segment after the bucket is required (no trailing slash-only paths). Examples:
+   - Valid: `s3://bucket/data`, `http://host:9000/bucket/data`
+   - Invalid: `s3://bucket/` (missing object_path), `s3:///bucket/data` (extra slash)

eloq_data_store_service/RocksDB_Configuration_Flags.md (2)

101-106: Clarify object_path is mandatory and no trailing slash.

State explicitly that the segment after the bucket must be non-empty; s3://bucket/ is invalid and will be rejected.

Apply:

 **URL Format Specification:**
-- S3: `s3://{bucket_name}/{object_path}`
+- S3: `s3://{bucket_name}/{object_path}` (object_path is required; no trailing slash-only paths)
 - HTTP/HTTPS: `http(s)://{host}:{port}/{bucket_name}/{object_path}`

---

116-117: Precedence note is good. Add bucket_prefix reminder.

Reiterate here that bucket_prefix is ignored when URL is used to avoid misconfiguration.

Apply:

 **Note:** If both `rocksdb_cloud_s3_url` and legacy options are provided, the URL-based configuration takes precedence and overrides the legacy settings.
+When URL-based config is active, any `rocksdb_cloud_bucket_prefix` value is ignored.

eloq_data_store_service/rocksdb_config.h (1)

118-120: noexcept on trivial accessor.

Mark IsS3UrlConfigured as noexcept.

-    bool IsS3UrlConfigured() const { return !s3_url_.empty(); }
+    bool IsS3UrlConfigured() const noexcept { return !s3_url_.empty(); }

eloq_data_store_service/rocksdb_cloud_dump.cpp (1)

72-96: Update usage to include s3_url and deprecations.

Help users discover the new flag and legacy status.

-        "--bucket_name=BUCKET --bucket_prefix=PREFIX [options]\n"
+        "[--s3_url=s3://BUCKET/PATH | --bucket_name=BUCKET --bucket_prefix=PREFIX] [options]\n"
 ...
-        "  --bucket_name=BUCKET         S3 bucket name\n"
-        "  --bucket_prefix=PREFIX       S3 bucket prefix\n"
-        "  --object_path=PATH           S3 object path for RocksDB Cloud "
+        "  --s3_url=URL                 URL form (recommended): s3://bucket/path or http(s)://host:port/bucket/path\n"
+        "  --bucket_name=BUCKET         Legacy: S3 bucket name\n"
+        "  --bucket_prefix=PREFIX       Legacy: S3 bucket prefix (ignored with --s3_url)\n"
+        "  --object_path=PATH           Legacy: S3 object path for RocksDB Cloud "
+        "  --s3_endpoint=URL            Legacy: Custom S3 endpoint URL (use URL host in --s3_url instead)\n"

eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)

378-386: Transfer Manager toggle is endpoint-driven; log rationale.

Optional: add a short log when disabling transfer manager due to custom endpoint to aid ops.

     if (!effective_endpoint_url.empty())
     {
         cfs_options_.s3_client_factory =
             BuildS3ClientFactory(effective_endpoint_url);
         // Intermittent and unpredictable IOError happend from time to
         // time when using aws transfer manager with minio. Disable aws
         // transfer manager if endpoint is set (minio).
         cfs_options_.use_aws_transfer_manager = false;
+        LOG(INFO) << "Custom S3 endpoint set; disabling AWS Transfer Manager.";
     }

eloq_data_store_service/rocksdb_config.cpp (3)

296-304: Clarify URL style and consider guarding the flag.

• Help text implies path-style addressing only; explicitly say virtual-hosted-style (e.g., https://bucket.s3.amazonaws.com/path) is not supported to avoid confusion.
• For consistency with other cloud flags, consider wrapping this DEFINE under the same S3/GCS build guard to hide it from non-cloud builds.

Apply this doc tweak (optional):

 DEFINE_string(rocksdb_cloud_s3_url,
               "",
-              "RocksDB cloud S3 URL. Format: s3://{bucket}/{path} or "
-              "http(s)://{host}:{port}/{bucket}/{path}. "
+              "RocksDB cloud S3 URL. Supported formats (path‑style only): "
+              "s3://{bucket}/{path} or http(s)://{host}[:port]/{bucket}/{path}. "
+              "Virtual‑hosted style (e.g., https://{bucket}.s3.{region}.amazonaws.com/{path}) is not supported. "
               "Examples: s3://my-bucket/my-path, "
               "http://localhost:9000/my-bucket/my-path. "
               "This option takes precedence over legacy configuration options "
               "if both are provided");

Optionally guard the flag:

+#if (defined(DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3) || \
+     defined(DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_GCS))
 DEFINE_string(rocksdb_cloud_s3_url,
 ...
               "if both are provided");
+#endif

---

307-399: Make URL parsing more robust (scheme normalization, slash handling, minor shadowing).

Parser works for the documented formats but can be hardened:

• Normalize scheme to lowercase to accept “S3/HTTP/HTTPS”.
• Trim redundant leading slashes after the endpoint split; strip trailing slashes from object_path.
• Avoid reusing the name first_slash in two scopes (small readability nit).

Apply minimally invasive changes:

 S3UrlComponents ParseS3Url(const std::string &s3_url)
 {
     S3UrlComponents result;

@@
-    result.protocol = s3_url.substr(0, protocol_end);
+    result.protocol = s3_url.substr(0, protocol_end);
+    // Normalize protocol to lowercase
+    std::transform(result.protocol.begin(),
+                   result.protocol.end(),
+                   result.protocol.begin(),
+                   [](unsigned char c) { return static_cast<char>(std::tolower(c)); });
@@
-    if (result.protocol == "http" || result.protocol == "https")
+    if (result.protocol == "http" || result.protocol == "https")
     {
         // Find the first slash after the host:port
-        size_t first_slash = remaining.find('/');
-        if (first_slash == std::string::npos)
+        size_t endpoint_sep = remaining.find('/');
+        if (endpoint_sep == std::string::npos)
         {
             result.error_message =
                 "Invalid S3 URL format: missing bucket and object path";
             return result;
         }

         // Store the full endpoint URL including protocol
-        result.endpoint_url =
-            result.protocol + "://" + remaining.substr(0, first_slash);
-        remaining = remaining.substr(first_slash + 1);
+        result.endpoint_url =
+            result.protocol + "://" + remaining.substr(0, endpoint_sep);
+        remaining = remaining.substr(endpoint_sep + 1);
+        // Remove any leading slashes before bucket
+        while (!remaining.empty() && remaining.front() == '/')
+        {
+            remaining.erase(0, 1);
+        }
     }
@@
-    size_t first_slash = remaining.find('/');
-    if (first_slash == std::string::npos)
+    size_t key_sep = remaining.find('/');
+    if (key_sep == std::string::npos)
     {
         result.error_message =
             "Invalid S3 URL format: missing object path (format: "
             "{bucket_name}/{object_path})";
         return result;
     }

-    result.bucket_name = remaining.substr(0, first_slash);
-    result.object_path = remaining.substr(first_slash + 1);
+    result.bucket_name = remaining.substr(0, key_sep);
+    result.object_path = remaining.substr(key_sep + 1);
+    // Normalize object_path: strip trailing slashes
+    while (!result.object_path.empty() && result.object_path.back() == '/')
+    {
+        result.object_path.pop_back();
+    }

Note: you’ll need #include <algorithm> and #include <cctype> at the top.

Please confirm S3UrlComponents default-initializes is_valid to false; otherwise set result.is_valid = false; on construction paths.

---

613-619: Precedence is read; enforce it and surface conflicts.

You read s3_url_ first (good). When both URL and legacy fields are set, ensure downstream code:

• Ignores legacy bucket_prefix_/bucket_name_/object_path_ and logs a clear warning, or
• Fails fast if both are present.

If not already enforced elsewhere, add a post-load check in this ctor after legacy fields are read:

if (!s3_url_.empty()) {
    if (!bucket_prefix_.empty()) {
        LOG(WARNING) << "rocksdb_cloud_bucket_prefix is ignored when rocksdb_cloud_s3_url is set.";
        bucket_prefix_.clear();
    }
    // Consider warning for bucket_name_/object_path_ too, since the URL is the source of truth.
}

Would you like a follow-up patch plus unit tests covering precedence and logging?

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

eloq_data_store_service/rocksdb_config.h (1)

92-177: Use distinct variable names for improved clarity.

The variable first_slash is declared at line 136 (inside the http/https block) and again at line 152 (after the block). While technically in different scopes, consider renaming to endpoint_slash and bucket_slash respectively for better readability.

Regarding other edge cases: Trailing slashes in URLs like s3://bucket/path/ will result in object_path="path/" with the trailing slash preserved. This appears to be expected behavior. URL-encoded characters (e.g., %20) don't appear to be supported by this parser, which aligns with the codebase design—URL decoding is expected to be handled by the caller before passing URLs to ParseS3Url.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)

299-334: Verify LOG(FATAL) behavior and improve error message.

The URL parsing and override logic is well-implemented with clear logging. However:

1. LOG(FATAL) terminates the process immediately - this is appropriate for invalid configuration at startup, but ensure this is the intended behavior for your deployment model.

2. Error message could be more actionable: Consider adding a suggestion about which configuration to check or remove:

         LOG(FATAL) << "Invalid rocksdb_cloud_s3_url: "
                    << url_components.error_message
                    << ". URL format: s3://{bucket}/{path} or "
                       "http(s)://{host}:{port}/{bucket}/{path}. "
                    << "Examples: s3://my-bucket/my-path, "
-                   << "http://localhost:9000/my-bucket/my-path";
+                   << "http://localhost:9000/my-bucket/my-path. "
+                   << "Please correct the rocksdb_cloud_s3_url configuration or remove it to use legacy configuration.";

eloq_data_store_service/rocksdb_cloud_dump.cpp (1)

52-53: Fix inconsistent flag description format.

The bucket_prefix flag description on line 53 should match the format of other legacy flags for consistency:

-DEFINE_string(bucket_prefix, "", "S3 bucket prefix (legacy, not supported with s3_url)");
+DEFINE_string(bucket_prefix, "", "S3 bucket prefix (legacy, use s3_url instead)");

The phrase "not supported with s3_url" might be confusing - it's better to consistently use "use s3_url instead" like the other flags.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro